Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Public key-based routing (Feature **NEEDS NUMBER***) #814

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

joostjager
Copy link
Collaborator

@joostjager joostjager commented Nov 3, 2020

In today's Lightning Network, routes are described as a series of short channel ids. Routing nodes use these short channel ids to determine to which of its peers an htlc needs to be forwarded.

The advantage of short channel ids is that they are short - just 8 bytes.

A great downside is that a short channel id corresponds directly to a channel point on-chain. A channel point reveals the size of the channel and is thereby an indication of the amount of funds that are kept by the channel parties in their node hot wallets. Large hot wallets are an attractive target to hackers.

For public channels, this isn't an issue. To advertise a public channel, the channel point needs to be revealed anyway so that other node are able to verify the existence of the channel.

For private channels it is different. Private channels are only disclosed in invoice route hints and don't need to be verified. The invoice is created and signed by the receiver and there is no reason to include a non-existing channel in there. Ideally the invoice does not contain channel points. Otherwise someone can request an invoice (or multiple invoices) from a service that accepts Lightning payments and inspect the route hints to gauge the service's node capacity.

This PR proposes a new routing mode based on full public keys instead of short channel ids. When nodes signal option_pubkey_routing, they'll also accept a public key in the tlv onion payload to control the outgoing direction of the htlc.

With this feature, receivers can strip the channel points of private channels from invoices for peers that support option_pubkey_routing. Senders need to construct pubkey tlv payloads for the final private leg towards the destination. This increases the payload size with an extra 25 bytes. But as this is only required for the last (or last few) hops, the reduction of the maximum route length isn't very significant.

Optionally the invoice format can be changed so that short channel ids can be omitted completely from route hints, saving 8 bytes per route hint.

Overall a relatively simple change that makes Lightning safer to use for network endpoints in particular.

@t-bast
Copy link
Collaborator

t-bast commented Nov 3, 2020

Why not work on reviewing #765 instead, which fixes that and adds other goodies?

@joostjager
Copy link
Collaborator Author

I like small steps.

@t-bast
Copy link
Collaborator

t-bast commented Nov 3, 2020

But the risk is cluttering the spec with things that quickly become obsolete...IMHO the spec is exactly where we want to spend time reviewing, get frustrated sometimes because things are moving slow (already 6 months since route blinding has been proposed), but end up with a solution that is great for everyone.

I think we don't want half-measures here, and leaking the node_id (which option_pubkey_routing does) is not good enough IMO.

@joostjager
Copy link
Collaborator Author

It depends on what problem you want to address. I am mainly concerned with safety. Don't tell anyone how many bitcoins you have. This PR fixes just that.

I totally agree that it would be nice to do more, but a larger change also carries more risk of getting pushed back.

Nevertheless thanks for sharing your view on the scope. I'd be interested to hear other people's opinions. Perhaps it turns out that the only purpose of this PR is to underline that we want to do full obfuscation right from the start.

@Roasbeef
Copy link
Collaborator

Roasbeef commented Nov 3, 2020

But the risk is cluttering the spec with things that quickly become obsolete...IMHO the spec is exactly where we want to spend time reviewing, get frustrated sometimes because things are moving slow (already 6 months since route blinding has been proposed), but end up with a solution that is great for everyone.

Solution here IMO is just to go with distinct documents in a more BIP-like format. We should really put an end to all the "conditional" statements in the spec as is. You can describe your proposal, get feedback, and ppl can implement it or not. Everyone has different priorities and resources they can allocate to changes they care about in the end. People only really care about changes that directly impact them, or that they have some sort of higher level need for, and in the end we have quite a bit of ways to extend the protocol as is.

@Roasbeef
Copy link
Collaborator

Roasbeef commented Nov 3, 2020

As an example, this can be a very small stand alone document that describes the motivation and what changes it requires (just a new TLV). That can then be linked from the routing section as additional features implementors may want to implement. This is nice also since it starts to separate the "base" protocol from other enhancements. The feature bit system lets clients know what they should and shouldn't include. In the end, things can move a lot more quickly, with people only opining on what they actually care about.

@t-bast
Copy link
Collaborator

t-bast commented Nov 4, 2020

Solution here IMO is just to go with distinct documents in a more BIP-like format.

I agree, that's already what has been done for route blinding and trampoline routing (at least on the format, and put in a proposals folder). What we haven't sorted out yet is the merge conditions.

@joostjager
Copy link
Collaborator Author

I can convert it to a separate proposal, but first wanted to see if there is enthusiasm for this change at all. I think that for just assessing that, a diff may even be better because it quickly shows where the change impact is.

@joostjager
Copy link
Collaborator Author

joostjager commented Nov 5, 2020

A poor man's version of pubkey routing could be to use the truncated pubkey as the channel id. Routing nodes would first interpret the channel id as a real id and if that fails try matching with the channel id as a truncated pubkey. Not my preference, but also no tlv changes needed.

@Roasbeef
Copy link
Collaborator

Roasbeef commented Nov 7, 2020

What we haven't sorted out yet is the merge conditions.

True, maybe we just need a 2 impl (as if one impl is doing something own their own, they doesn't necessarily need a spec?) sign off? Alternatively, if only a single impl is doing something, still likely nice to have good documentation/rationale for it in case others want to implement them as well. For that we could just fallback to like a "hard reject, do no harm" type rule. If we want to refer to the "extensions" numerically, we could do what ETH does and name them after the fact using the PR number itself.

@t-bast
Copy link
Collaborator

t-bast commented Nov 9, 2020

True, maybe we just need a 2 impl (as if one impl is doing something own their own, they doesn't necessarily need a spec?) sign off?

That sounds good to me:

  • 1 implementation doing its thing deserves an open PR to let others know how they're doing it and have the opportunity to get feedback from other implementations
  • 2 implementations doing it means it's probably ok for a merge for this kind of optional features

@cdecker
Copy link
Collaborator

cdecker commented Nov 16, 2020

As an example, this can be a very small stand alone document that describes the motivation and what changes it requires (just a new TLV). That can then be linked from the routing section as additional features implementors may want to implement. This is nice also since it starts to separate the "base" protocol from other enhancements. The feature bit system lets clients know what they should and shouldn't include. In the end, things can move a lot more quickly, with people only opining on what they actually care about.

Totally agree, this was the main reason I started pushing for separate proposal documents, that walk through the rationale and contain all the relevant pieces in one place in order to catch up with a proposal, and for the living spec to reference for further details.

Ulimately, the spec should be a document that reads like an implementation, without all the discussion fluff, and the individual proposals should be self-contained documents shedding light on individual aspects of that implementation.

@@ -262,6 +262,9 @@ This is a more flexible format, which avoids the redundant `short_channel_id` fi
2. data:
* [`32*byte`:`payment_secret`]
* [`tu64`:`total_msat`]
1. type: 10 (`pub_key`)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about outgoing_node_id?
I already have added this field in the trampoline proposal (I believe also with TLV type 10) so I'm interested in getting it spec-ed anyway :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that it is a good idea to have two unapproved prs reference each other. Suppose this PR would be approved first, then renaming the field is easy enough and won't change the wire protocol.

09-features.md Outdated
@@ -39,6 +39,7 @@ The Context column decodes as follows:
| 16/17 | `basic_mpp` | Node can receive basic multi-part payments | IN9 | `payment_secret` | [BOLT #4][bolt04-mpp] |
| 18/19 | `option_support_large_channel` | Can create large channels | IN | | [BOLT #2](02-peer-protocol.md#the-open_channel-message) |
| 20/21 | `option_anchor_outputs` | Anchor outputs | IN | `option_static_remotekey` | [BOLT #3](03-transactions.md) |
| 22/23 | `option_pubkey_routing` | Public key routing | IN | `var_onion_optin` | [BOLT #4](04-onion-routing.md#tlv-payload-format) |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the flags be IN9?
It feels like you need to set it to required on your invoice when you're using it, otherwise senders will not correctly include the outgoing_node_id field in the onion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is that senders recognize the all-zeroes channel id and use this as a cue to switch to pubkey routing. But for older senders this will just lead to failures that an invoice feature bit can prevent. Added the 9.

@rustyrussell
Copy link
Collaborator

rustyrussell commented Mar 1, 2021

Feature number clash with #824

Can we move this to 26/27 as I've suggested in the title?

@rustyrussell rustyrussell changed the title Public key-based routing Public key-based routing (Feature 24/25?) Mar 1, 2021
@rustyrussell rustyrussell changed the title Public key-based routing (Feature 24/25?) Public key-based routing (Feature 26/27?) Mar 1, 2021
@joostjager joostjager changed the title Public key-based routing (Feature 26/27?) Public key-based routing (Feature 26/27) Mar 2, 2021
@joostjager
Copy link
Collaborator Author

Feature bits updated

@niftynei
Copy link
Collaborator

This now clashes with #672. I'd suggest to move to 30/31?

@joostjager
Copy link
Collaborator Author

Maybe I missed something, but what's the point of mentioning non-final feature bits in the title @rustyrussell ?

@cdecker
Copy link
Collaborator

cdecker commented Mar 17, 2021

Maybe I missed something, but what's the point of mentioning non-final feature bits in the title @rustyrussell ?

It's there so we can detect these clashes early on, before people start implementing and signalling online.

@rustyrussell rustyrussell changed the title Public key-based routing (Feature 26/27) Public key-based routing (Feature **NEEDS NUMBER***) Sep 22, 2021
@rustyrussell
Copy link
Collaborator

Yeah, I just noticed that this clashes with the merged option for anysegwit. Please renumber if this is still a desired change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants